Select rows and symbols in walkVM by attribution address - #813

…resolution

resolveNativeFrameForWalkVM uses two addresses for two jobs: range lookups key
off the attribution address so a call that is the last instruction of its
caller still resolves to the caller, while the emitted pc_offset keeps deriving
from the raw pc because its meaning is a wire contract rather than a local
detail. Neither half was gated by a test.

Both are now pinned at a synthetic zero-gap symbol boundary, the shape where
the two addresses disagree: flagging the pc as a return address must move the
resolved symbol from the following function back to the caller, and must leave
the emitted offset untouched. A third case pins that an address well inside a
function resolves identically either way, so the adjustment cannot be read as a
blanket shift.

Both assertions were mutation-checked -- reverting the lookup to the raw pc
fails the boundary case alone, and deriving pc_offset from the attribution
address fails the wire case alone.

The fixtures publish synthetic CodeCaches through a new test-only entry point
rather than the test binary's own symbols, which is what makes a controlled
zero-gap boundary possible at all and keeps these tests free of the
GNU-as/ELF-CFI and updateSymbols() dependencies that confine
returnAddressAttribution_ut to Linux.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

walkVM tracked "is this pc a return address?" in a bool sitting beside the pc,
so the two could drift: a newly added `pc = ...` inherited whatever the
previous frame had set, silently attributing the next lookup to the wrong
address. That is the exact hazard WalkPc was introduced for in walkFP/walkDwarf,
and walkVM is the default cstack path, so it is the one that most needed it.

walkVM now drives a WalkPc. Every pc source states what it produced --
setReturnAddress for a saved-pc slot or link register, setRecoveredPc for a
DWARF row, setExactAddress where the arch unwind helpers already folded the
adjustment in -- and consumers read raw() or attribution() explicitly. Adding a
pc source is now a compile-time decision instead of something a reviewer has to
notice.

Threading the seed through surfaced a real gap: walkVM is entered either from a
ucontext, whose pc is the exact interrupted address, or from callerPC(), which
is a genuine return address on every architecture where
CALLER_PC_IS_RETURN_ADDRESS holds. Both arrived as a bare pointer and were
treated as exact, so the callerPC() entry never got the adjustment it needed.
The private overload now takes that distinction from its caller, the same way
walkFP and walkDwarf seed themselves.

No behaviour change otherwise: the mutator chosen at each site reproduces the
flag that site already set.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

… the call

walkVM's public entry point built its two starting states inline, so the pc and
the flag describing it were separate arguments chosen a few tokens apart in two
near-identical call expressions. Nothing bound them together, which is the same
drift the walk itself now avoids by carrying a WalkPc.

Both states are one named value instead. walkVMSeed() decides where a walk
starts -- register set and the nature of the pc together -- and the entry point
delegates without a branch of its own.

The frame no longer outlives the decision: its pc/sp/fp are copied out as
scalars, which is all the callee ever used, and the ucontext they point into
outlives the walk regardless.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Three review follow-ups.

addLibraryForTest shipped in every build. Every other test hook in this
codebase is either behind #ifdef UNIT_TEST or reached through a friend
*TestAccessor, so a publicly callable mutator for the process-wide library set
was both inconsistent and present where nothing should be able to call it. It
is now compiled only into gtest binaries, which is where its one caller lives.

prev_native_pc lost its last reader when the walk started carrying the previous
frame as a WalkPc; the declaration stayed behind. Unused-variable warnings are
not errors here, so nothing caught it.

RemoteSymbolication.md still described the two-argument
resolveNativeFrameForWalkVM. It now names the third parameter and says what it
selects -- lookups move to the attribution address, the emitted pc_offset does
not -- since the wire value being unchanged is the part a reader of that
document needs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

The three arch unwind helpers were all treated as having already applied the
attribution adjustment. That holds on x86_64, which subtracts one inside the
helper, but not on aarch64: every branch walkVM can reach there assigns the
link register or a saved-pc slot, both raw return addresses. The one aarch64
branch that does subtract is guarded by `&pc == &this->pc()`, true only on the
AsyncGetCallTrace path where the caller passes the frame's own pc rather than a
local, so walkVM never takes it.

A sample leaving a generated frame for a native caller whose call is the last
instruction before a symbol or FDE boundary was therefore still attributed to
whatever follows, on arm64, for exactly the frames this change set out to fix.

recordUnwoundPc() names the difference in one place. x86_64 behaviour is
unchanged; the distinction disappears once the helpers agree on a contract.

Reported by Codex review on #813.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

walkVM read its VMThread through JVMThread::current(), which asserts its
thread-local key is live -- something only a JVM attach sets up. Nothing below
that line needs the thread on the native path, and every use of it is already
NULL-guarded, so it now asks whether one exists instead of requiring it. With a
JVM attached the answer is always yes and nothing changes; without one the walk
takes the same degraded path it would on an unattached thread, and
WALKVM_NO_VMTHREAD still records that it did.

That is the whole of what stood between walkVM and a unit test. VMStructs stays
uninitialised, so CodeHeap::contains() is false for every address and the walk
goes down the native branch on every frame -- which is the branch the
attribution work touches.

The test plants one synthetic linked frame so the walk steps off an exact
ucontext leaf onto a caller whose pc comes out of a return-address slot. Both
land on the same zero-gap boundary, so they must resolve to different functions
purely because of that difference. Mutating the DWARF recovery to claim its pc
is exact fails it, reporting the following function where the caller belongs.

Two platform details the fixture has to respect, both found the hard way:
uc_mcontext is a pointer on Darwin and embedded on Linux, so a zeroed
ucontext_t needs storage to point at or the first register access faults in a
loop behind the profiler's own SIGSEGV handler; and the frame layout has to
come from FrameDesc::default_frame, which is what a table-less CodeCache
returns, not fallback_default_frame(), which differs from it on Apple aarch64.

This covers the DWARF recovery path only. The saved-pc slots in the compiled
and stub arms sit behind CodeHeap::contains() and stay unreachable here.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…step

61959b0 reconciled three gaps between walkVM's copy of the DWARF unwind
step and walkDwarf's, all of them in the pc/fp recovery half. A mechanical
diff of the two bodies shows the CFA half still diverges in two places, and
unlike the earlier three these resolve in opposite directions -- so the
extraction that follows could not have been the pure move that commit
claimed it was setting up.

walkVM falls through on a CFA register it does not implement.

walkDwarf ends its cfa_reg chain with an else that stops the walk. walkVM
instead pre-screened DW_REG_INVALID and anything past DW_REG_PLT, then ran
the same three arms with no else. With DW_REG_FP 6, DW_REG_SP 7 and
DW_REG_PLT 128 on x86_64, a register in 0..5 or 8..127 matches no arm and is
not rejected: sp keeps the value it had on entry to the step, and the walk
reads the caller's pc and fp out of the current frame's own slots.

It is reachable from ordinary DWARF. DwarfParser takes the register straight
off the wire under DW_CFA_def_cfa, _def_cfa_register and _def_cfa_sf, and
addRecord() only rejects it above 0xFF, so nothing narrows it to the three
the walkers implement -- a function whose CFA is based on any other register
lands in the hole. WalkVmAlsoStopsOnAnUnhandledCfaRegister plants exactly
that row and shows walkVM recording a second frame off an unmoved sp where
walkDwarf stops at the leaf.

walkVM's pre-screen is subsumed by the new else and goes away with it; every
value it rejected now falls out of the same arm as the rest.

walkDwarf trusts a frame pointer walkVM checks.

The other direction: walkVM sanity-checks fp for bounds and alignment before
deriving a CFA from it, walkDwarf did not. A misaligned fp whose row offset
re-aligns the result slips past every later test, all of which look at sp,
so only a check on fp itself stops it. 61959b0's rule was that each gap is
taken from walkDwarf; this one goes the other way, and the comment travels
with the code so the reason is not lost.

Both changes are gated, each verified to fail with its fix reverted:

  WalkDwarfStopsOnAnUnhandledCfaRegister      baseline, passes throughout
  WalkVmAlsoStopsOnAnUnhandledCfaRegister     gates the else
  WalkDwarfRejectsAMisalignedFramePointer     gates the fp check

Two properties of the fixture are load-bearing and commented as such:
DW_PC_OFFSET is 1, so fp_off must be even or the step takes the DW_OP_breg
arithmetic branch instead of the memory-slot one under test; and cfa_off must
be non-zero or the aarch64 defaultSenderSP() branch fires. A DW_REG_INVALID
row above each planted leaf stops whichever walker does step past it, so a
failure reports a wrong depth rather than running into unplanted memory.

With this the two bodies differ only in where attribution_pc is declared and
in walkVM's prev_native_walk_pc/have_prev_native_pc bookkeeping, which stays
at the call site. advanceDwarfFrame() can now be extracted as a pure move.

Full native suite green on linux-x64 in debug and release, and this binary
green in all four configurations. That is also the first Linux run of the
walkVM fixtures, which had only been exercised on macOS/arm64.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…rame

walkDwarf and walkVM carried two copies of the same unwind step. 61959b0
and 06ab5b6 reconciled the five differences between them; this moves what
is left into one always_inline helper in dwarfStep.inline.h and calls it from
both. 183 lines of duplication become 10.

always_inline is load-bearing, not an optimisation. AddressSanitizer
instruments per function after inlining, so an inlined body takes its
caller's sanitizer attribute. walkVM is no_sanitize("address") because it
reads arbitrary stack memory while sampling; walkDwarf is deliberately
instrumented, and both have been that way since before the two were split
apart in #451. Out of line, one attribute would have to serve both -- either
dropping walkDwarf's instrumentation or adding to walkVM the instrumentation
the attribute exists to avoid. Measured on the built objects rather than
assumed: with always_inline the helper emits no out-of-line symbol at all,
walkVM still contains zero __asan references and walkDwarf still contains
its own.

What moved and what did not:

  - Both callers keep their own `bottom`, computed from their own frame, and
    pass it in.
  - `depth` is a parameter because the two callers disagree on it. walkVM's
    fp-chain fallback reaches the step through `goto dwarf_unwind`, which
    skips its fillFrame(frames[depth++], ...), so on that path it arrives one
    lower than walkDwarf does at the same logical position. Passing it
    through is what keeps this a move rather than a merge; the header says so
    at the parameter.
  - The lookup goes inside, so the helper takes Profiler* rather than the
    CodeCache* the ticket sketched: the DW_REG_PLT arm needs attribution_pc
    to decide the stub offset, so a caller-side lookup would have to
    recompute walk_pc.attribution() anyway.
  - walkVM's prev_native_walk_pc/have_prev_native_pc bookkeeping stays at the
    call site, hoisted to just above the call from the middle of the step.
    Nothing between those two points mutates walk_pc, and a stop leaves the
    loop, so the only reader -- the MARK_THREAD_ENTRY check on the next
    iteration -- cannot tell the difference.

Each of the nine break sites becomes a `return false`; the count was checked
mechanically before and after rather than by eye, since four of them sit on
arms no test in the suite reaches.

Equivalence was checked against the disassembly, not only the tests.
walkDwarf's call sequence is identical instruction-for-instruction in
release; walkVM's call multiset is identical with one static-guard pair
reordered. Neither is byte-identical -- inlining a function changes what the
scheduler and register allocator see, so release moves 313 -> 308
instructions in walkDwarf and 2997 -> 3061 in walkVM. The sanitizer configs
compile at -O0, where the helper's parameters and its FrameDesc copy
materialise as real stack traffic, so walkDwarf grows there (1020 -> 1173
instructions, 9 -> 25 instrumented accesses). Those configs are test-only;
the shipped release build is the one above.

Full native suite green on linux-x64 in debug and release, and the three
binaries over this code green under ASan and TSan.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

…test fails

Revert before merge. Diagnostic only, no product change.

gtestDebug_walkVmAttribution_ut fails on every aarch64 cell and has since
66eea0c, while amd64 is green in every configuration. Which assertion
fails is not recoverable from the logs: the Gradle daemon swallows the test
binary's stdout, so the job reports only the task name. .gitlab/sanitizer-tests
already works around this by using Gradle for compile+link and then running
each binary straight from the shell; this does the same for one binary on the
glibc-aarch64 job.

Placed before the Test step, which would otherwise fail the job first, and
marked continue-on-error so it can only add output.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>